-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Building support for decoupling Dash from Flask and supporting Quart / FastAPI servers #3430
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
work to modularize the dash eco-system and decouple from Flask
∙ - added types to BaseFactory to remove linting errors on create app in Flask and Quart Factory
… `request` context
* ∙ Added has_request_context to base_server ∙ removed flask specific import in _validate and use backends.backend.has_request_context now * ∙ added context to request adapter ∙ callback context uses request adapter context now ∙ * added get_root_path to dash _utils removed flask.helpers.get_root_path usage * moved compress to server implementations flask fully decoupled from dash * fixed compress in quart and fastapi * Fixed server.config in fastapi to use config file * Update dash/backends/_fastapi.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * removed unused flask import in pages * Update dash/backends/_quart.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * removed flask specific return type to remove global dependency --------- Co-authored-by: Christian Giessel <cgiessel@tesla.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
T4rk1n
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good, a few fixes are needed and I left a few suggestion.
- The serve_health route in dash.py still using flask.Response directly and need to be added in the backends.
- There is a few Flask references left in comments on the Dash class that need to be changed.
- The fastapi app is not running in debug mode if the app filename name is not "app.py".
| """ | ||
|
|
||
| def wrap(func: _t.Callable[[], _f.Response]): | ||
| def wrap(func: _t.Callable[[], _t.Any]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be a future pain point to update existing hooks, maybe we can try adding a flask.Response converter in the adapter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have the self.backend.make_response which uses the server adapters.
| _backend_imports = { | ||
| "flask": ("dash.backends._flask", "FlaskDashServer"), | ||
| "fastapi": ("dash.backends._fastapi", "FastAPIDashServer"), | ||
| "quart": ("dash.backends._quart", "QuartDashServer"), | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should have a way for this to be customizable, maybe add a hook for that. Then the backend library can have the hook inside it's init.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can bring your own server by passing it in the Dash spin up, this is just to import them from the pre-configured backends.
| @property # pragma: no cover - interface | ||
| @abstractmethod | ||
| def cookies(self): | ||
| raise NotImplementedError() | ||
|
|
||
| @property # pragma: no cover - interface | ||
| @abstractmethod | ||
| def headers(self): | ||
| raise NotImplementedError() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can type those as dict, then make sure the backends returns a dict copied from all the cookies/headers.
In the same way, maybe we can add a single get_cookie(...) and get_header(...) for single usage.
| @abstractmethod | ||
| def make_response( | ||
| self, data, mimetype=None, content_type=None | ||
| ) -> Any: # pragma: no cover - interface | ||
| pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The usual usecase for the response in the dash context is to set cookies/headers, maybe we should a global set_cookie(...) and set_header akin to set_props that can be used across the different backends with ease.
dash/backends/_fastapi.py
Outdated
| if kwargs.get("reload"): | ||
| # Dynamically determine the module name from the file path | ||
| file_path = frame.filename | ||
| spec = spec_from_file_location("app", file_path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need the real file here, when I try with an app not named app with debug=True I get:
INFO: Uvicorn running on http://127.0.0.1:8050 (Press CTRL+C to quit)
INFO: Started reloader process [80425] using StatReload
ERROR: Error loading ASGI app. Could not import module "app".
WARNING: StatReload detected changes in 'quart_app.py'. Reloading...
ERROR: Error loading ASGI app. Could not import module "app".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I adjusted it to where if you run in debug=True, you need to have server exposed as the variable. This should make it more consistent.
dash/backends/_fastapi.py
Outdated
| include_in_schema=False, | ||
| ) | ||
|
|
||
| def dispatch(self, dash_app: Dash): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be a good time to rename this to a better name like serve_callback.
dash/backends/_quart.py
Outdated
| if Response is None: | ||
| raise RuntimeError("Quart not installed; cannot generate Response") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be happening, we should raise back the import error with a custom message to install dash[quart].
dash/backends/_quart.py
Outdated
| return Response( | ||
| pkgutil.get_data("dash", "favicon.ico"), content_type="image/x-icon" | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the same for all the backends, maybe it be in the base and just call the proper make_response to handle that?
| def serve_component_suites( | ||
| self, dash_app: Dash, package_name: str, fingerprinted_path: str | ||
| ): # noqa: ARG002 unused req preserved for interface parity | ||
| path_in_pkg, has_fingerprint = check_fingerprint(fingerprinted_path) | ||
| _validate.validate_js_path(dash_app.registered_paths, package_name, path_in_pkg) | ||
| extension = "." + path_in_pkg.split(".")[-1] | ||
| mimetype = mimetypes.types_map.get(extension, "application/octet-stream") | ||
| package = sys.modules[package_name] | ||
| dash_app.logger.debug( | ||
| "serving -- package: %s[%s] resource: %s => location: %s", | ||
| package_name, | ||
| getattr(package, "__version__", "unknown"), | ||
| path_in_pkg, | ||
| package.__path__, | ||
| ) | ||
| data = pkgutil.get_data(package_name, path_in_pkg) | ||
| headers = {} | ||
| if has_fingerprint: | ||
| headers["Cache-Control"] = "public, max-age=31536000" | ||
|
|
||
| if Response is None: | ||
| raise RuntimeError("Quart not installed; cannot generate Response") | ||
| return Response(data, content_type=mimetype, headers=headers) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function will mostly be the same across backends with a difference in the Response type, in this case we are missing the etag stuff that might need to be adapted.
I think maybe we could have a Response type that is dash specific and then we have a ResponseAdapter like we do for the RequestAdapter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm for changing this, but there are quite a few differences between the Responses, etc.
I like your idea for Response information, I'm just not sure that its necessary for this as there may be more things that popup inside of this process for different servers that need to be handled inside of this function.
# Conflicts: # dash/_callback.py # dash/dash.py
Co-authored-by: Philippe Duval <t4rk@outlook.com>
…ing-your-own-server
…se` instead of splitting it out
This is an open PR draft, to contribute please target my forked branch.
The goal of this PR is to modularize the Dash setup to be independent of Flask (will fallback to Flask) and allow devs to configure their own backend.
fixes #1571